Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes MSVC (MSVC2019, ...) errors when projects are used which have _CRT_SECURE_NO_WARNINGS already set as part of the default build settings #72

Closed
wants to merge 1 commit into from

Conversation

GerHobbelt
Copy link

fixes for MSVC (MSVC2019, ...) and when projects are used which have _CRT_SECURE_NO_WARNINGS already set as part of the default build settings.


Testing

This was tested as part of a larger work (other PRs are forthcoming shortly) after hunting down shutdown issues (application lockups, etc.) in a large application.

Tested this code via your provided test code rig; see my own fork and the referenced commits which point into there.

Tested on AMD Ryzen 3700X, 128GB RAM, latest Win10/64, latest MSVC2019 dev environment. Using in-house project files which use a (in-house) standardized set of optimizations.

Additional information

TBD

The patches are hopefully largely self-explanatory. Where deemed useful, the original commit messages from the dev fork have been referenced and included.

…_CRT_SECURE_NO_WARNINGS already set as part of the default build settings.
@bshoshany
Copy link
Owner

Thanks for this pull request! This is a good idea.

In the next release, I plan to stop using std::localtime, and this should eliminate the need to define _CRT_SECURE_NO_WARNINGS altogether. However, if I find that reproducing the results of std::localtime in a thread-safe way (while still only using standard C++17) requires too much effort, I will incorporate your proposed change into the code.

Also, may I ask why you're adding #undef min and #undef max?

@bshoshany bshoshany closed this Aug 23, 2022
@GerHobbelt
Copy link
Author

Sure.

Collision between std::min and std::max and min and max defines in windows.h (or one of the header files it pulls in), when I use the threadpool in another codebase, where those #includes have already been loaded before the thread-pool code is #included.

https://stackoverflow.com/questions/5004858/why-is-stdmin-failing-when-windows-h-is-included#answer-5004874, but NOMINMAX was, shall we say, not an option either. 😢 Yeah.

@bshoshany
Copy link
Owner

Interesting... I never use Windows.h, so I never encountered this issue before. But I just checked and found that changing const int64_t offset = std::min(random_start, static_cast<int64_t>(random_end)); to const int64_t offset = std::min<int64_t>(random_start, static_cast<int64_t>(random_end)); also resolves this issue, while still being 100% portable and not adding any extra lines, so I think that's what I'll do instead.

@GerHobbelt
Copy link
Author

👍

@bshoshany
Copy link
Owner

Update: the solution for the collision with Windows.h is now implemented in v3.4.0. The collision with _CRT_SECURE_NO_WARNINGS is something I'm still working on and should be implemented (one way or another) in the next release.

@bshoshany
Copy link
Owner

Update: I spent some time looking into portable thread-safe alternatives to std::localtime and could not find any (std::chrono was expanded for this purpose in C++20, but this project is C++17-compatible, so I cannot use that). So I will just incorporate your solution of only setting _CRT_SECURE_NO_WARNINGS if it is not already defined. This will be implemented in the upcoming v3.5.0 release.

@bshoshany
Copy link
Owner

Update: This issue has been fixed in v3.5.0, which was just released. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants